Conversation
…etails Change MockImpl, MockFactory, and DelegateFactory generated types from `internal` to `file` scoped, keeping them out of user IntelliSense. Merge MockImpl + MockFactory into a single generated file so they can reference each other while both being file-scoped. Update the static extension to use Mock.Of<T> instead of direct factory calls.
There was a problem hiding this comment.
Review: refactor(mocks): use file-scoped types for generated implementation details
This is a well-executed, focused refactor. The goal is clear and the implementation is consistent across all generated types. Here are my observations:
What works well
- Using
file-scoped types forMockImpl,MockFactory, andMockDelegateFactoryis an excellent ergonomics improvement. These are pure implementation details and have no business leaking into user IntelliSense. The C# 11filemodifier is the right tool here. - Merging
MockImplandMockFactoryinto a single generated file (_MockImplFactory.g.cs) is a clean design: both types must be in the same file to reference each other, so co-locating them removes the need to coordinate across two separately emitted files. - Converting
MockFactoryBuilder.Build()/MockImplBuilder.Build()from returningstringtovoid BuildInto(CodeWriter writer, ...)and extractingBuildCombinedImplAndFactoryintoMockGeneratoris a good separation of concerns: the builders no longer need to know about file headers or namespaces, and the generator owns the file structure. - Removing the
// Generate constructor dispatch based on available constructorscomment (now that the guard logic is in a named helper) keeps the code self-documenting. - Routing
MockStaticExtensionthroughMock.Of<T>()instead of the now-invisibleXxxMockFactory.Create(...)is the correct fix; the extension was the only remaining external caller that needed to be updated.
Potential issue: [ModuleInitializer] on internal static void Register() inside a file class
The [ModuleInitializer] attribute requires that the method be accessible from the module-level initializer, meaning it must be public or internal (C# spec §22.5; the Roslyn requirement is the method is "accessible from the module-level initializer"). Using internal on the Register() method inside a file-scoped class is technically valid — the compiler generates an internal-equivalent symbol scoped to the file — and the attribute does work here. The snapshot tests passing on net10.0 confirm this at runtime. However, it is worth noting this is slightly unusual; public would also be acceptable and might be more idiomatic for [ModuleInitializer] methods. Not a bug, but worth a comment if any reader ever wonders whether internal is intentional or a leftover from before file was added.
Observation: GenerateWrapMock now delegates entirely to GenerateImplFactoryMembersAndEvents
GenerateWrapMock is now a one-liner:
private static void GenerateWrapMock(SourceProductionContext spc, MockTypeModel model)
{
var fileName = GetSafeFileName(model);
GenerateImplFactoryMembersAndEvents(spc, model, fileName);
}This is fine, but if GenerateWrapMock does nothing beyond computing fileName and forwarding, it could simply be inlined into the call site in Initialize. That said, keeping it as a named method improves readability at the dispatch site, so this is a style preference only.
Observation: Wrap_Mock_Filters_Internal_Virtual_Members_From_External_Assembly snapshot reordering
In the diff for this snapshot, the MockMemberExtensions class moved to the end of the file and the WrapMockImpl/WrapMockFactory pair moved to the top. This is a side-effect of combining impl+factory into one file emitted first, then the members builder emitted separately. The logical grouping (impl + factory together) is now better than the old ordering (factory in one file, impl in another), so the reorder is a net improvement.
Minor: BOM removal in some .verified.txt snapshots
Several verified files lost their leading UTF-8 BOM (// <auto-generated/>). This is a cosmetic change from the header generation in BuildCombinedImplAndFactory not emitting a BOM that the old per-builder approach apparently did. Not a functional issue, but worth being aware of if snapshot tooling (Verify.Net) ever re-adds it on regeneration.
Summary
The change is correct, consistent, and an improvement in all dimensions: better user experience (implementation types hidden from IntelliSense), fewer generated files to reason about, and cleaner builder APIs. The 746 tests passing and all 25 snapshots updated confirm the mechanical correctness. No blocking concerns.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
Summary
MockImpl,MockFactory, andDelegateFactorygenerated types frominternaltofilescoped, removing them from user IntelliSense entirelyMockImpl+MockFactoryinto a single generated file (_MockImplFactory.g.cs) so both can befilescoped while still referencing each otherMockStaticExtensionto route throughMock.Of<T>()instead of directly calling the now-invisible factoryGenerateImplFactoryMembersAndEvents()helper inMockGeneratorto reduce duplicationTypes now file-scoped (invisible to users)
{Name}MockImpl_MockImplFactory.g.cs{Name}WrapMockImpl_MockImplFactory.g.cs{Name}MockFactory/WrapMockFactory/PartialMockFactory_MockImplFactory.g.cs{Name}MockDelegateFactory_MockDelegateFactory.g.csNot changed (with reasoning)
StaticEngine— sharedAsyncLocalstate referenced across_MockBridge.g.csand_MockImplFactory.g.cs; must remaininternal(already has[EditorBrowsable(Never)])Mockwrapper,MockMemberExtensions,MockCall,MockEvents,MockStaticExtension,Mockablebridge) — part of the fluent API surfaceTest plan
.verified.txtfiles